Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Data providers tests for vip3 #2431

Merged
merged 41 commits into from
Jan 31, 2024
Merged

Data providers tests for vip3 #2431

merged 41 commits into from
Jan 31, 2024

Conversation

0xverin
Copy link
Contributor

@0xverin 0xverin commented Jan 26, 2024 •
edited
Loading

Context

resolves p-366

Usage example:

  • If you want to test a specific VC(vip3-membership-card-gold), you can simply input its ID on the command line, like: pnpm --filter integration-tests run test-data-provider:local --id=vip3-membership-card-gold.(you can find the ID in vc jsonfile

  • Testing parameters(i.e. mockAddress or expectedCredentialValue) can be directly modified in the *.json files.
    I feel this approach is relatively maintainable. If it seems reasonable, I'll add the others as well. This PR currently provides four.

If you have a better solution, please feel free to share it at any time.

@linear Linear
Copy link

linear bot commented Jan 26, 2024

P-366 Add new VC test for VIP3

@0xverin 0xverin changed the title Data providers tests for vip3 [WIP]:Data providers tests for vip3 Jan 26, 2024
@0xverin 0xverin changed the title [WIP]:Data providers tests for vip3 Data providers tests for vip3 Jan 26, 2024
@0xverin 0xverin requested a review from a team January 26, 2024 08:53
kziemianek
kziemianek reviewed Jan 26, 2024
View reviewed changes
Copy link
Member

@kziemianek kziemianek left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain how and why we are testing data provieders through typescript tests ?
Or maybe we are just testing request vc cli commands ? 🙃
I think there should be no thing like dataproviders in ts code, it introduces unnecessary coupling.

tee-worker/ts-tests/integration-tests/data-providers.test.ts Outdated Show resolved Hide resolved
@0xverin
Copy link
Contributor Author

0xverin commented Jan 26, 2024

Could you explain how and why we are testing data provieders through typescript tests ?

We indeed need real accounts and a real endpoint to test the value of VCs. To be precise, we are assisting IdHub in identifying issues with VCs—for example, cases where an real account should meet the conditions for generating a credential, but IdHub receives a false value. This has occurred before.

How: We might need the CI to use the data-provider's key (expect VIP3).

Or maybe we are just testing request vc cli commands ?

CLI help creating an ID graph with a real account. It ensures that Alice meets the conditions for being an ETH holder, and when requesting a VC from the data-provider, it should return a value of true.

cc @kziemianek

Kailai-Wang
Kailai-Wang reviewed Jan 27, 2024
View reviewed changes
Copy link
Collaborator

@Kailai-Wang Kailai-Wang left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't read thoroughly, but I feel it's a bit overcomplicated in general.

Can't we have some testdata.json which includes:

{
  [
     "idgraph": [
       "did1",
       "did2",
     ],
     "assertion": "a1",
     "expectedValue": true
   ],
   [
   ],
   ...
}

The script just iterates through the testdata, construct idgraph and request vc to see if we get the expected result.

Isn't that more clear and simple?
Just my naive thought - I could be wrong

@0xverin
Copy link
Contributor Author

0xverin commented Jan 29, 2024 •
edited
Loading

I haven't read thoroughly, but I feel it's a bit overcomplicated in general.

Can't we have some testdata.json which includes:

{
  [
     "idgraph": [
       "did1",
       "did2",
     ],
     "assertion": "a1",
     "expectedValue": true
   ],
   [
   ],
   ...
}

The script just iterates through the testdata, construct idgraph and request vc to see if we get the expected result.

Isn't that more clear and simple? Just my naive thought - I could be wrong

I can modify it into a JSON file; the effect will be the same.

Traf333
Traf333 reviewed Jan 29, 2024
View reviewed changes
Copy link
Contributor

@Traf333 Traf333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked only source code and later will try to run everything locally. Put some comments.
@Verin1005 let me know what do you think about it?

tee-worker/ts-tests/integration-tests/package.json Show resolved Hide resolved
tee-worker/ts-tests/integration-tests/data-providers.test.ts
assert.equal(events.length, 1);
}

async function requestVc(id: string, index: number) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to utilise this function for other scenarios, like NFT holder or BNB domain?

Copy link
Contributor Author

@0xverin 0xverin Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're referring to other cases, right? Currently, this pr is for VIP3, but I am testing the other cases. To avoid ambiguity, I am redefining credentials array here.

tee-worker/ts-tests/integration-tests/data-providers.test.ts
} = new URL(process.env.WORKER_ENDPOINT!);
const { protocol: nodeProtocal, hostname: nodeHostname, port: nodePort } = new URL(process.env.NODE_ENDPOINT!);

async function linkIdentityViaCli(id: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to utilise this function for other VC scenarios, like NFT holder or BNB domain?

tee-worker/ts-tests/integration-tests/data-providers.test.ts Outdated Show resolved Hide resolved
tee-worker/ts-tests/integration-tests/data-providers.test.ts Outdated Show resolved Hide resolved
@0xverin 0xverin requested a review from Traf333 January 30, 2024 10:20
Traf333
Traf333 approved these changes Jan 30, 2024
View reviewed changes
Copy link
Contributor

@Traf333 Traf333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks @Verin1005 for initiating this big topic as it will definitely save us time for a long run

@0xverin 0xverin merged commit 0b690cd into dev Jan 31, 2024
34 checks passed
@0xverin 0xverin deleted the vip3-vc-tests branch January 31, 2024 03:46
@Traf333 Traf333 mentioned this pull request Feb 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kziemianek kziemianek kziemianek left review comments

@Traf333 Traf333 Traf333 approved these changes

@jonalvarezz jonalvarezz Awaiting requested review from jonalvarezz

@BillyWooo BillyWooo Awaiting requested review from BillyWooo

@grumpygreenguy grumpygreenguy Awaiting requested review from grumpygreenguy

@higherordertech higherordertech Awaiting requested review from higherordertech

@outofxxx outofxxx Awaiting requested review from outofxxx

@Kailai-Wang Kailai-Wang Awaiting requested review from Kailai-Wang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@0xverin @kziemianek @Traf333 @Kailai-Wang